-
Notifications
You must be signed in to change notification settings - Fork 106
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Specify Callback Argument Type in init Functions in ODE solvers (2N, 3Sstar, PairedExplicitRK, SSP) #2026
Specify Callback Argument Type in init Functions in ODE solvers (2N, 3Sstar, PairedExplicitRK, SSP) #2026
Conversation
Review checklistThis checklist is meant to assist creators of PRs (to let them know what reviewers will typically look for) and reviewers (to guide them in a structured review process). Items do not need to be checked explicitly for a PR to be eligible for merging. Purpose and scope
Code quality
Documentation
Testing
Performance
Verification
Created with ❤️ by the Trixi.jl community. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2026 +/- ##
==========================================
+ Coverage 96.23% 96.25% +0.02%
==========================================
Files 462 462
Lines 37084 37076 -8
==========================================
Hits 35687 35687
+ Misses 1397 1389 -8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Thanks, could you maybe also make more precise error messages regarding the continuous callbacks similar to #2008 (comment)? Also I think it would be better to |
@JoshuaLampert By throwing an ArgumentError, do you mean by bringing back
Would that really be necessary since by this we are bringing back the redundant |
…tegration methods
…/warisa-r/Trixi.jl into specify_init_arg_type_integrator
Yes, you are totally right. I didn't want to bring back the |
Got it! Thank you for the explanation. |
…tegration methods
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! What about
Trixi.jl/src/time_integration/methods_SSP.jl
Lines 158 to 167 in 9d8aac9
if callback isa CallbackSet | |
foreach(callback.continuous_callbacks) do cb | |
error("unsupported") | |
end | |
foreach(callback.discrete_callbacks) do cb | |
cb.initialize(cb, integrator.u, integrator.t, integrator) | |
end | |
elseif !isnothing(callback) | |
error("unsupported") | |
end |
I think the SSP method was also not covered in #1975. Is there a reason for that? |
Co-authored-by: Joshua Lampert <51029046+JoshuaLampert@users.noreply.github.com>
Co-authored-by: Joshua Lampert <51029046+JoshuaLampert@users.noreply.github.com>
You are right in that SSP method isn't covered at all. I personally am also not sure why but I just left it out since I wasn't instructed to make any changes there. But according to the issue #1886 s title, SSP method should also been updated right? @DanielDoehring |
Hm the SSP one is a bit different as it heavily relies on stage callbacks, which is not supported by the the standard So from my side this looks alright! |
Ok, if SSP is conceptually different to the others it's probably fine to have ignored it in #1975, but from what I can tell at least the changes from this PR also apply to the SSP method as well. At least the code block from this comment is pretty much the same as the corresponding code block in the other integrators. Thus, if you don't object @DanielDoehring I would also opt for applying the changes from this PR to the SSP method in order to keep consistency. |
Yeah I guess we should make the error messages more similar. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks!
This pull request improves type safety and clarity in the
init
functions of various integrator methods that are updated per issue #1886 by specifying the argument type for callback, according to @JoshuaLampert suggestion in #2008. The callback parameter, which can either be aCallbackSet
orNothing
, is now explicitly typed asUnion{CallbackSet, Nothing}
.